Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix broken build flag, move build to one directory #2442

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix broken build flag, move build to one directory #2442

wants to merge 5 commits into from

Conversation

sam-herman
Copy link
Contributor

Description

Addresses a bug and house cleaning during build.

  1. Cmake has invalid syntax in if condition causing this message:
CMake Error at cmake/init-faiss.cmake:114 (if):
  if given arguments:

    "AND" "NOT" "" "STREQUAL" ""

  Unknown arguments specified
Call Stack (most recent call first):
  CMakeLists.txt:107 (include)
  1. Move all build artifacts created into a build directory following conventional build standards. Currently there are multiple temporary artifacts generated by cmake which are scattered under jni directory.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • [ x] New functionality includes testing.
  • [x ] New functionality has been documented.
  • [x ] API changes companion pull request created.
  • [ x] Commits are signed per the DCO using --signoff.
  • [ x] Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@sam-herman sam-herman force-pushed the fix-syntax-typo-breaking-build branch from 3f12185 to 515c986 Compare January 25, 2025 04:02
@navneet1v navneet1v mentioned this pull request Jan 27, 2025
Closed
5 tasks
@sam-herman
Copy link
Contributor Author

@navneet1v @vamshin @jmazanec15 can someone review this fix? Also, I am not sure why the 2 BWC checks are failing and can't re-run the workflows to get them to see if it's a transient things. Can you help me try re-running those?

@navneet1v
Copy link
Collaborator

@navneet1v @vamshin @jmazanec15 can someone review this fix? Also, I am not sure why the 2 BWC checks are failing and can't re-run the workflows to get them to see if it's a transient things. Can you help me try re-running those?

Triggered a re run.

@navneet1v
Copy link
Collaborator

@sam-herman can you rebase your code from main and then we can trigger a run again. I see some changes have been made in main branch which fixes some issues.

@sam-herman sam-herman force-pushed the fix-syntax-typo-breaking-build branch from 25c356a to 53943ec Compare January 28, 2025 22:34
@sam-herman
Copy link
Contributor Author

@sam-herman can you rebase your code from main and then we can trigger a run again. I see some changes have been made in main branch which fixes some issues.

@navneet1v looks like after the rebase there are even more tests failing, including some of the non-bwc ones. maybe try re-run to check if it's transient?

@navneet1v
Copy link
Collaborator

@sam-herman i think for main branch since 3.0 is in progress there are a lot of breaking changes.

@navneet1v
Copy link
Collaborator

Tagging 3.0 release owner here to know when the main branch will be stable since a lot of prs are struck.

@Vikasht34

jmazanec15
jmazanec15 reviewed Jan 29, 2025
View reviewed changes
jni/cmake/init-faiss.cmake
@@ -111,10 +111,10 @@ endif()
if(NOT DEFINED AVX512_SPR_ENABLED)
# Check if the system is Intel(R) Sapphire Rapids or a newer-generation processor
execute_process(COMMAND bash -c "lscpu | grep -q 'GenuineIntel' && lscpu | grep -i 'avx512_fp16' | grep -i 'avx512_bf16' | grep -i 'avx512_vpopcntdq'" OUTPUT_VARIABLE SPR_FLAGS OUTPUT_STRIP_TRAILING_WHITESPACE)
if (AND NOT "${SPR_FLAGS}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @sam-herman . @mulugetam Can you check this and make sure this change makes sense?

jmazanec15
jmazanec15 reviewed Jan 29, 2025
View reviewed changes
build.gradle
@@ -333,7 +333,7 @@ task cmakeJniLib(type:Exec) {
workingDir 'jni'
def args = []
args.add("cmake")
args.add(".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good update. This makes cleaning some much easier. That being said, in the buildJniLib, instead of changing workingDir, can we just do:

make -Cbuild

I worry if we add "/" we might end up having issues with path resolution on different platforms (unless gradle handles this for us)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The windows build seems to work fine https://github.com/opensearch-project/k-NN/actions/runs/13020810027/job/36325044749?pr=2442

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - think thats fine

@jmazanec15
Copy link
Member

thanks @sam-herman. main is messed up right now due to lucene upgrade. On other PRs, weve been developing directly on 2.x and backporting to main to unblock (#2438 )

@sam-herman
Copy link
Contributor Author

@navneet1v @Vikasht34 Any updates on fixes to BWC and integ tests?

@Vikasht34
Copy link
Collaborator

@sam-herman , Tentative changes to be merged in main by End of Next week .

@jmazanec15
Copy link
Member

@sam-herman I think this looks good - if you want to expedite, you can change branch merging to to 2.x and then we can backport to main.

@sam-herman
move build to one directory, fix broken flag
c62b4ee 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman
fix make path
4431ea4 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman
changelog update
5d61402 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman sam-herman requested a review from Vikasht34 as a code owner February 7, 2025 18:41
@sam-herman sam-herman force-pushed the fix-syntax-typo-breaking-build branch from f09882e to 0b86cea Compare February 7, 2025 23:51
@sam-herman sam-herman force-pushed the fix-syntax-typo-breaking-build branch from 0b86cea to dd9a595 Compare February 7, 2025 23:52
@sam-herman
fix make discovery for gradle
c57ea42 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman
Copy link
Contributor Author

@jmazanec15 @navneet1v I added a couple of fixes for issues I just spotted in main:

  1. I noticed that recently after changes on main the build fails on :buildJni by failing to find cmake and make unless used with full path. I added a fix in the gradle.build to auto find the full path and use it in the args.
  2. There was a classpath change for org.opensearch.client to org.opensearch.transport that broke compilation.

Everything is building now locally and working fine, however now all the integ tests of the build in the GitHub workflow seems to be failing to start a cluster even though it all works and passes locally. Do you know what could be the reason?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmazanec15 jmazanec15 jmazanec15 left review comments

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@ryanbogan ryanbogan Awaiting requested review from ryanbogan ryanbogan is a code owner

@luyuncheng luyuncheng Awaiting requested review from luyuncheng luyuncheng is a code owner

@shatejas shatejas Awaiting requested review from shatejas shatejas is a code owner

@0ctopus13prime 0ctopus13prime Awaiting requested review from 0ctopus13prime 0ctopus13prime is a code owner

@Vikasht34 Vikasht34 Awaiting requested review from Vikasht34 Vikasht34 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sam-herman @navneet1v @jmazanec15 @Vikasht34